Skip to content

feat: void element tags in helpers are selectable between > and /> #6717

Closed
ddevsr wants to merge 3 commits intocodeigniter4:4.3from
ddevsr:selfclosing-html
Closed

feat: void element tags in helpers are selectable between > and /> #6717
ddevsr wants to merge 3 commits intocodeigniter4:4.3from
ddevsr:selfclosing-html

Conversation

@ddevsr
Copy link
Copy Markdown
Collaborator

@ddevsr ddevsr commented Oct 19, 2022

Description
Fixes #6649
Supersedes and closes #6658

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@ddevsr
Copy link
Copy Markdown
Collaborator Author

ddevsr commented Oct 19, 2022

@kenjis PHPStan error will be appear if changes typedoc in form_checkbox to array|string. Maybe you can fix in PR #6715

@kenjis kenjis added the 4.3 label Oct 19, 2022
@kenjis
Copy link
Copy Markdown
Member

kenjis commented Oct 19, 2022

@ddevsr Okay, I will take care of that.

I sent a PR #6721
PHPStan is correct.
Offset 'checked' on the array does not exist at the line.

Comment thread tests/system/Helpers/FormHelperTest.php
@kenjis
Copy link
Copy Markdown
Member

kenjis commented Oct 20, 2022

Is this a breaking change or not?
See #6658 (comment)

@ddevsr
Copy link
Copy Markdown
Collaborator Author

ddevsr commented Oct 20, 2022

Is this a breaking change or not? See #6658 (comment)

For XHTML is breaking change, but analytics say 94,8% using HTML in 2022

See https://w3techs.com/technologies/history_overview/markup_language/ms/y

@totoprayogo1916
Copy link
Copy Markdown
Contributor

If only, the list here can be used for the default form_helper.

public $list = [
'xhtml11' => '<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN" "http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">',
'xhtml1-strict' => '<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">',
'xhtml1-trans' => '<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">',
'xhtml1-frame' => '<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Frameset//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-frameset.dtd">',
'xhtml-basic11' => '<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML Basic 1.1//EN" "http://www.w3.org/TR/xhtml-basic/xhtml-basic11.dtd">',
'html5' => '<!DOCTYPE html>',
'html4-strict' => '<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN" "http://www.w3.org/TR/html4/strict.dtd">',
'html4-trans' => '<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3.org/TR/html4/loose.dtd">',
'html4-frame' => '<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Frameset//EN" "http://www.w3.org/TR/html4/frameset.dtd">',
'mathml1' => '<!DOCTYPE math SYSTEM "http://www.w3.org/Math/DTD/mathml1/mathml.dtd">',
'mathml2' => '<!DOCTYPE math PUBLIC "-//W3C//DTD MathML 2.0//EN" "http://www.w3.org/Math/DTD/mathml2/mathml2.dtd">',
'svg10' => '<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.0//EN" "http://www.w3.org/TR/2001/REC-SVG-20010904/DTD/svg10.dtd">',
'svg11' => '<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">',
'svg11-basic' => '<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1 Basic//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11-basic.dtd">',
'svg11-tiny' => '<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1 Tiny//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11-tiny.dtd">',
'xhtml-math-svg-xh' => '<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1 plus MathML 2.0 plus SVG 1.1//EN" "http://www.w3.org/2002/04/xhtml-math-svg/xhtml-math-svg.dtd">',
'xhtml-math-svg-sh' => '<!DOCTYPE svg:svg PUBLIC "-//W3C//DTD XHTML 1.1 plus MathML 2.0 plus SVG 1.1//EN" "http://www.w3.org/2002/04/xhtml-math-svg/xhtml-math-svg.dtd">',
'xhtml-rdfa-1' => '<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML+RDFa 1.0//EN" "http://www.w3.org/MarkUp/DTD/xhtml-rdfa-1.dtd">',
'xhtml-rdfa-2' => '<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML+RDFa 1.1//EN" "http://www.w3.org/MarkUp/DTD/xhtml-rdfa-2.dtd">',
];

Comment thread system/Helpers/form_helper.php Outdated
@kenjis
Copy link
Copy Markdown
Member

kenjis commented Oct 20, 2022

doctype([$type = 'html5'])
...
Helps you generate document type declarations, or DTD’s. HTML 5 is used by default, but many doctypes are available.
https://codeigniter4.github.io/CodeIgniter4/helpers/html_helper.html#doctype

@kenjis
Copy link
Copy Markdown
Member

kenjis commented Oct 20, 2022

To begin with, HTML5 is already obsolete.
This list is only used with doctype().
Why don't we make doctype() deprecate?

@ddevsr
Copy link
Copy Markdown
Collaborator Author

ddevsr commented Oct 21, 2022

To begin with, HTML5 is already obsolete. This list is only used with doctype(). Why don't we make doctype() deprecate?

For other HTML5, doctype() is useful and very simple

@ddevsr
Copy link
Copy Markdown
Collaborator Author

ddevsr commented Oct 21, 2022

I already added dynamic appear HTML Tag with setting property endlineTag document type by user.

@kenjis kenjis added the enhancement PRs that improve existing functionalities label Oct 22, 2022
Comment thread app/Config/DocTypes.php Outdated
Comment thread system/Common.php Outdated
Comment thread system/Common.php
Comment thread system/Common.php
@kenjis kenjis added the docs needed Pull requests needing documentation write-ups and/or revisions. label Oct 22, 2022
Comment thread system/Common.php
Comment thread app/Config/DocTypes.php
Comment thread system/Common.php
@kenjis kenjis added the breaking change Pull requests that may break existing functionalities label Oct 24, 2022
@kenjis kenjis changed the title refactor: make it valid schema html in form_helper feat: void element tags in helpers are selectable between > and /> Oct 24, 2022
Comment thread app/Config/DocTypes.php
Comment thread app/Config/DocTypes.php
@ddevsr ddevsr requested review from kenjis and paulbalandan October 25, 2022 09:33
Comment thread app/Config/DocTypes.php Outdated
Comment thread app/Config/DocTypes.php Outdated
Comment thread system/Common.php Outdated
Comment thread tests/system/CommonFunctionsTest.php Outdated
Comment thread tests/system/Helpers/FormHelperTest.php
Comment thread user_guide_src/source/changelogs/v4.3.0.rst Outdated
@totoprayogo1916
Copy link
Copy Markdown
Contributor

Btw, this also needs the same touch
Maybe in this PR or another PR

return '<input type="hidden"' . (! empty($id) ? ' id="' . esc($id, 'attr') . '"' : '') . ' name="' . csrf_token() . '" value="' . csrf_hash() . '" />';

return '<meta' . (! empty($id) ? ' id="' . esc($id, 'attr') . '"' : '') . ' name="' . csrf_header() . '" content="' . csrf_hash() . '" />';

@ddevsr
Copy link
Copy Markdown
Collaborator Author

ddevsr commented Oct 25, 2022

Btw, this also needs the same touch Maybe in this PR or another PR

return '<input type="hidden"' . (! empty($id) ? ' id="' . esc($id, 'attr') . '"' : '') . ' name="' . csrf_token() . '" value="' . csrf_hash() . '" />';

return '<meta' . (! empty($id) ? ' id="' . esc($id, 'attr') . '"' : '') . ' name="' . csrf_header() . '" content="' . csrf_hash() . '" />';

Thankyou for suggest

Comment thread system/Common.php Outdated
*
* @internal
*/
function solidus(): string
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about renaming to _solidus()?
HTML helper has _get_uri().

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I propose to rename it to _solidus().

  1. This is not a normal helper function.
  2. Now it does output a space and a solidus or empty string.

Comment thread tests/system/Helpers/FormHelperTest.php
Comment thread user_guide_src/source/changelogs/v4.3.0.rst Outdated
@kenjis
Copy link
Copy Markdown
Member

kenjis commented Oct 27, 2022

This PR changes helpers outputs by default:

<input type="checkbox" name="foo" value="bar" />

<input type="checkbox" name="foo" value="bar">

Please add it to

  1. changelog BREAKING
    https://github.com/codeigniter4/CodeIgniter4/blob/4.3/user_guide_src/source/changelogs/v4.3.0.rst#others
  2. upgrade guide that how to keep the compatible outputs as before
    https://github.com/codeigniter4/CodeIgniter4/blob/4.3/user_guide_src/source/installation/upgrade_430.rst#breaking-enhancements

Comment thread system/Helpers/form_helper.php Outdated
Comment thread user_guide_src/source/installation/upgrade_430.rst Outdated
Comment thread user_guide_src/source/installation/upgrade_430.rst Outdated
Comment thread user_guide_src/source/changelogs/v4.3.0.rst Outdated
Comment thread user_guide_src/source/changelogs/v4.3.0.rst Outdated
@kenjis kenjis added stale Pull requests with conflicts and removed docs needed Pull requests needing documentation write-ups and/or revisions. labels Oct 27, 2022
Copy link
Copy Markdown
Member

@kenjis kenjis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix the output when using HTML5:
<input type="hidden" name="foo" value="bar" >

<input type="hidden" name="foo" value="bar">

@ddevsr ddevsr requested a review from kenjis October 30, 2022 05:14
@codeigniter4 codeigniter4 deleted a comment from martinemily3 Oct 30, 2022
Comment thread tests/system/Helpers/FormHelperTest.php Outdated
@kenjis
Copy link
Copy Markdown
Member

kenjis commented Oct 30, 2022

You used git merge: 15ec83c
But please use git rebase instead to update the PR branch.
See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#updating-your-branch

@kenjis kenjis removed the stale Pull requests with conflicts label Oct 30, 2022
refactor: make it valid schema html

refactor: make it valid schema HTML

refactor: make it valid schema HTML
Update system/Common.php

Co-authored-by: kenjis <kenji.uui@gmail.com>

refactor: make it valid HTML in form_helper and html_helper
@ddevsr ddevsr requested a review from kenjis October 30, 2022 08:17
Copy link
Copy Markdown
Member

@paulbalandan paulbalandan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you git rebase -i and combine similar commits together? It would be hard later on debugging where to check on this git history.

Comment thread system/Common.php Outdated
Comment on lines +566 to +572
if (! function_exists('solidus')) {
/**
* Generates the solidus character (`/`) depending on the HTML5 compatibility flag in `Config\DocTypes`
*
* @internal
*/
function solidus(): string
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (! function_exists('solidus')) {
/**
* Generates the solidus character (`/`) depending on the HTML5 compatibility flag in `Config\DocTypes`
*
* @internal
*/
function solidus(): string
if (! function_exists('_solidus')) {
/**
* Generates the solidus character (`/`) depending on the HTML5 compatibility flag in `Config\DocTypes`
*
* @internal
*/
function _solidus(): string

@ddevsr
Copy link
Copy Markdown
Collaborator Author

ddevsr commented Oct 30, 2022

Can you git rebase -i and combine similar commits together? It would be hard later on debugging where to check on this git history.

Fail rebase, how to back to commit 17d66d62cf57a46851aff15963904484c91b222e?

@ddevsr
Copy link
Copy Markdown
Collaborator Author

ddevsr commented Oct 30, 2022

I will going with new PR

@ddevsr ddevsr closed this Oct 30, 2022
@ddevsr ddevsr reopened this Oct 30, 2022
@kenjis
Copy link
Copy Markdown
Member

kenjis commented Oct 30, 2022

This commit a8efae2 seems to have nothing to do with Rector error.

This commit f7bea58 has duplicated commit messages.

@ddevsr ddevsr closed this Oct 30, 2022
@ddevsr ddevsr deleted the selfclosing-html branch October 30, 2022 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking change Pull requests that may break existing functionalities enhancement PRs that improve existing functionalities

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants